Skip to content

fix: tell review agents which project root to read files from - #363

Merged
nhorton merged 3 commits into
mainfrom
fix/worktree-review-file-paths
Apr 11, 2026
Merged

fix: tell review agents which project root to read files from#363
nhorton merged 3 commits into
mainfrom
fix/worktree-review-file-paths

Conversation

@nhorton

@nhorton nhorton commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • In git-worktree setups, reviewer subagents were reading files from the main checkout instead of the worktree where the commits actually live, producing spurious findings against stale content (e.g., a flood of file-purpose-comment MUST FAIL hits on files that already had purpose comments in the worktree).
  • Root cause: the @filepath entries in each instruction file's "Files to Review" section are not auto-expanded — the reviewer must Read each file itself — so they resolve against whatever cwd the subagent inherits. When that cwd is the main checkout, Read returns un-refactored content, which then disagrees with the "Relevant File Contents" block that was correctly inlined from the worktree.
  • Fix: emit an explicit ## Project Root directive at the top of each instruction file stating the absolute project root and telling the reviewer to prepend it when calling Read. Reviewer behavior is now cwd-independent.

Changes

  • src/deepwork/review/instructions.pybuild_instruction_file takes an optional project_root: Path; emits the new section when provided. write_instruction_files threads it through.
  • specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md — adds REVIEW-REQ-005.1.9 with the worktree rationale.
  • tests/unit/review/test_instructions.py — new tests covering presence, ordering before "Review Instructions", backward-compat when project_root is omitted, and the write_instruction_files plumbing.

Test plan

  • uv run pytest tests/unit/review/test_instructions.py (60 passed)
  • uv run pytest tests/unit/review/ (260 passed)
  • Verify the generated instruction file contains a visible ## Project Root section in an actual worktree run

🤖 Generated with Claude Code

nhorton and others added 3 commits April 11, 2026 12:36
In git-worktree setups, reviewer subagents were reading files from the
main checkout instead of the worktree where the commits actually live,
producing spurious findings against stale (un-refactored) content. The
"@filepath" entries in the instruction file's "Files to Review" section
are not auto-expanded — the reviewer must Read each file itself — so
they were being resolved against whatever cwd the subagent happened to
inherit.

Emit an explicit "## Project Root" directive at the top of each
instruction file stating the absolute project root and instructing the
reviewer to prepend it when calling Read. Adds REVIEW-REQ-005.1.9 and
tests validating presence, ordering, and the write_instruction_files
plumbing that threads project_root through.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-existing code-quality issues surfaced by the reviewers on this PR
(none were introduced by the original fix, but pulling them in here
since the files are already under review):

instructions.py:
- _run_precompute_command docstring no longer claims "command" is an
  absolute path — it's a shell command string, invoked via shell=True
- Noted the trust model for precompute shell commands (repo-sourced,
  never fed untrusted input)
- ThreadPoolExecutor now caps at max_workers=8 so a repo with many
  distinct precompute commands can't briefly fork dozens of shells
- _build_reference_files_section no longer re-encodes the (possibly
  truncated) content string to recompute its byte count; the consumed
  budget is now tracked explicitly as either original_byte_len or the
  exact remaining budget on truncation

config.py:
- ReviewRule.precomputed_info_bash_command field comment matches the
  docstring fix above

test_instructions.py:
- Moved five traceability comment blocks from inside the method body
  to the two lines directly preceding the def line, matching the
  convention enforced by test_file_quality
- test_count_cap_triggers_omission no longer hardcodes "f20.txt" as
  "one of the omitted" — derives the filename from MAX_INLINE_FILES
  so the test stays correct if the constant changes
- _sanitize_for_id import moved from a function-local line to the
  top-of-module import block

REVIEW-REQ-005 spec:
- Split five compound requirements so each numbered item carries
  exactly one RFC 2119 keyword (requirements_file DeepSchema rule).
  Preserved existing IDs (5.1.4, 5.1.8, 5.8.4–5.8.6) — each retains
  its primary MUST — and appended the secondary obligations as new
  sequential IDs (5.1.10, 5.1.11, 5.8.7–5.8.11)

CHANGELOG.md:
- Added entry for the worktree file-path fix under the Unreleased
  Fixed section

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
instructions.py:
- precomputed_info lookup now uses direct key access ([]) instead of
  .get() so a dict/filter drift between unique_commands and the per-task
  lookup would fail loudly instead of silently dropping precomputed
  context. Also aligned both predicates on `is not None` so an
  empty-string command would not be executed then silently discarded.
- Added a docstring note to _content_hash pinning the trust contract:
  files entries must be repo-root-relative paths from trusted
  .deepreview config, with no absolute paths or .. traversal.

test_instructions.py:
- New module-level `instructions_dir` pytest fixture that creates
  `.deepwork/tmp/review_instructions/` under tmp_path. Seven tests now
  use it instead of repeating the two-line setup inline.
- test_single_file_produces_expected_format uses direct
  `(tmp_path / "src").mkdir(...)` instead of the indirect `.parent.mkdir`
  form, matching adjacent tests in TestComputeReviewId.
- Added an inline comment on the `2 * MAX_INLINE_FILES` fence-count
  assertion explaining the 2x relationship.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nhorton
nhorton added this pull request to the merge queue Apr 11, 2026
Merged via the queue into main with commit 484e1d6 Apr 11, 2026
5 checks passed
@nhorton
nhorton deleted the fix/worktree-review-file-paths branch April 11, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant